Correctly free relevant scripts when closing scene tabs #86008

Delsin-Yu · 2023-12-10T17:58:08Z

Fixes #85983
EditorNode::_remove_scene from editor/editor_node.cpp calls editor_data.clear_script_icon_cache() for cleaning all script icon cache, previously, the implementation inside _remove_edited_scene(p_change_tab) loops through all scene tabs including the one we are closing, results in a call to EditorData::get_script_icon() on the closing scene tab, and create Ref<Script>s to that unloading scene lives inside _script_icon_cache.

This issue causes, primarily, the c-sharp scripts not to get freed when closing scene tabs, and might be a cause of #78513.
These wild script references may live inside the ScriptManagerBridge.ScriptTypeBiMap, resulting in the duplicate key issue when unloading the assebly.

modules/mono/glue/runtime_interop.cpp:1324 - System.ArgumentException: An item with the same key has already been added. Key: Game.WeaponAbilityData
     at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
     at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
     at Godot.Bridge.ScriptManagerBridge.ScriptTypeBiMap.Add(IntPtr scriptPtr, Type scriptType) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.types.cs:line 23
     at Godot.Bridge.ScriptManagerBridge.AddScriptBridge(IntPtr scriptPtr, godot_string* scriptPath) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 419

AThousandShips · 2023-12-10T18:00:57Z

You should target master not 4.2, changes can then be cherry picked.

If you need more information about the update process please see: here. If wanted I can fix the branch for you.

AThousandShips

Some style fixes (will help as you need to push again to cause CI to run)

editor/editor_node.cpp

editor/gui/editor_scene_tabs.cpp

Delsin-Yu · 2023-12-10T18:13:36Z

I don't think adding this removing_scene field inside editor_data is healthy, a better approach would directly modify the logic inside EditorNode::_remove_edited_scene(bool p_change_tab) since we just need to find a way for calling editor_data.remove_scene(old_index) before _set_current_scene(new_index). However, the program started crashing after modifying the logic there.

Original Logic

void EditorNode::_remove_edited_scene(bool p_change_tab) {
	// When scene gets closed no node is edited anymore, so make sure the editors are notified before nodes are freed.
	hide_unused_editors(SceneTreeDock::get_singleton());

	int new_index = editor_data.get_edited_scene();
	int old_index = new_index;

	if (new_index > 0) {
		new_index = new_index - 1;
	} else if (editor_data.get_edited_scene_count() > 1) {
		new_index = 1;
	} else {
		editor_data.add_edited_scene(-1);
		new_index = 1;
	}

	if (p_change_tab) {
		_set_current_scene(new_index);
	}
	editor_data.remove_scene(old_index);
	_update_title();
	scene_tabs->update_scene_tabs();
}

New Logic that Causes Crashing

void EditorNode::_remove_edited_scene(bool p_change_tab) {
	// When scene gets closed no node is edited anymore, so make sure the editors are notified before nodes are freed.
	hide_unused_editors(SceneTreeDock::get_singleton());

	int old_index = editor_data.get_edited_scene();

	editor_data.remove_scene(old_index);

	if (p_change_tab) {
		int new_index = editor_data.get_edited_scene();

		if (new_index == 0) {
			if (editor_data.get_edited_scene_count() == 0) {
				editor_data.add_edited_scene(-1);
			}
		} else {
			new_index = new_index - 1;
		}

		_set_current_scene(new_index);
	}

	_update_title();
	scene_tabs->update_scene_tabs();
}

The Crash Message

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.2.1.rc.mono.custom_build (daeb1c7292cbb426fd45c5ca98b1c7da40b390ba)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] Node::get_configuration_warnings_as_string (D:\UnityProject\godot\scene\main\node.cpp:3067)
[1] SceneTreeEditor::_add_nodes (D:\UnityProject\godot\editor\gui\scene_tree_editor.cpp:287)
[2] SceneTreeEditor::_update_tree (D:\UnityProject\godot\editor\gui\scene_tree_editor.cpp:617)
[3] call_with_variant_args_helper<SceneTreeEditor,bool,0> (D:\UnityProject\godot\core\variant\binder_common.h:308)
[4] call_with_variant_args_dv<SceneTreeEditor,bool> (D:\UnityProject\godot\core\variant\binder_common.h:451)
[5] MethodBindT<SceneTreeEditor,bool>::call (D:\UnityProject\godot\core\object\method_bind.h:337)
[6] Object::callp (D:\UnityProject\godot\core\object\object.cpp:775)
[7] Callable::callp (D:\UnityProject\godot\core\variant\callable.cpp:69)
[8] CallQueue::_call_function (D:\UnityProject\godot\core\object\message_queue.cpp:222)
[9] CallQueue::flush (D:\UnityProject\godot\core\object\message_queue.cpp:328)
[10] SceneTree::physics_process (D:\UnityProject\godot\scene\main\scene_tree.cpp:473)
[11] Main::iteration (D:\UnityProject\godot\main\main.cpp:3596)
[12] OS_Windows::run (D:\UnityProject\godot\platform\windows\os_windows.cpp:1474)
[13] widechar_main (D:\UnityProject\godot\platform\windows\godot_windows.cpp:182)
[14] _main (D:\UnityProject\godot\platform\windows\godot_windows.cpp:204)
[15] main (D:\UnityProject\godot\platform\windows\godot_windows.cpp:218)
[16] WinMain (D:\UnityProject\godot\platform\windows\godot_windows.cpp:232)
[17] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[18] <couldn't map PC to fn name>
-- END OF BACKTRACE --

KoBeWi · 2023-12-14T15:09:44Z

I checked why the tab updates while deleting and seems like when closing the tab, update_scene_tabs() is called at least 3 times, for no reason. I can't reproduce the issue, so I can't test, but #83957 might fix it too.

The fix here looks like a workaround for bad tab management.

Delsin-Yu · 2023-12-15T02:54:19Z

I checked why the tab updates while deleting and seems like when closing the tab, update_scene_tabs() is called at least 3 times, for no reason. I can't reproduce the issue, so I can't test, but #83957 might fix it too.

The fix here looks like a workaround for bad tab management.

Detailed reproduction steps are written in issue #85983.
I just want to emphasize that this specific fix is targeting the CSharp script system (un-freed (re-referenced) script reference causing issue & module crash on assembly reload), and I agree that #83957 is a better way as long as that resolves the CSharp scripting issue as well.

KoBeWi · 2023-12-15T13:25:16Z

Detailed reproduction steps are written in issue

I know, I just don't have a C# setup to test. I tried with GDScript and other resources and it doesn't seem to happen.

I agree that #83957 is a better way as long as that resolves the CSharp scripting issue as well.

It's already merged, please give it a test.

Delsin-Yu · 2023-12-16T21:07:17Z

It's already merged, please give it a test.

The test indicates the issue persists to exist.
It looks like the core of this issue is not calling update_scene_tabs() multiple times, but calling update_scene_tabs() in the middle of deleting a tab will cause a re-reference of script dependencies.

Delsin-Yu · 2023-12-16T22:02:24Z

This is a flowchart for explaining this issue.

KoBeWi · 2023-12-16T22:06:51Z

Well the issue is still updating the tabs before the old tab was completely removed. So I think a better solution would be delaying tab update.

Delsin-Yu · 2023-12-17T11:53:14Z

Well the issue is still updating the tabs before the old tab was completely removed. So I think a better solution would be delaying tab update.

I will try that

editor/editor_node.cpp

KoBeWi

The tab update now happens after tab is fully removed, so if it fixes the issue, then it's fine.

On unrelated note, I think we should be more conservative with calling tab updates. There are cases like call to _set_current_scene() followed by update_scene_tabs(), which makes the update happen twice, because _set_current_scene() already does this internally. Tab update is now cheaper, but it's still called more than necessary.

YuriSizov · 2023-12-18T12:31:23Z

Could you please squash your commits into one? Make sure that the final commit has a short but descriptive message (the title of this PR is a good option). See this documentation, if you need help with squashing.

YuriSizov · 2023-12-18T17:25:25Z

Thanks, and congrats on your first merged Godot contribution!

This comment was marked as outdated.

Sign in to view

Delsin-Yu changed the title ~~4.2~~ Fixes Script of a Scene Dependency Not Freed when Closing Scene Tab under Certain Condition Dec 10, 2023

AThousandShips added bug topic:editor topic:dotnet labels Dec 10, 2023

AThousandShips added this to the 4.3 milestone Dec 10, 2023

AThousandShips added the needs work label Dec 10, 2023

Delsin-Yu force-pushed the 4.2 branch from a5deef9 to 268eb15 Compare December 10, 2023 18:04

Delsin-Yu requested review from a team as code owners December 10, 2023 18:04

AThousandShips removed request for a team December 10, 2023 18:04

AThousandShips removed the needs work label Dec 10, 2023

AThousandShips reviewed Dec 10, 2023

View reviewed changes

editor/editor_node.cpp Outdated Show resolved Hide resolved

editor/editor_node.cpp Outdated Show resolved Hide resolved

editor/gui/editor_scene_tabs.cpp Outdated Show resolved Hide resolved

AThousandShips requested review from KoBeWi and YuriSizov December 10, 2023 18:08

YuriSizov changed the title ~~Fixes Script of a Scene Dependency Not Freed when Closing Scene Tab under Certain Condition~~ Fix script of a scene dependency not freed when closing a scene tab Dec 11, 2023

Delsin-Yu force-pushed the 4.2 branch from d8bd010 to bd1d0d7 Compare December 17, 2023 13:54

AThousandShips reviewed Dec 17, 2023

View reviewed changes

editor/editor_node.cpp Outdated Show resolved Hide resolved

KoBeWi approved these changes Dec 17, 2023

View reviewed changes

YuriSizov changed the title ~~Fix script of a scene dependency not freed when closing a scene tab~~ Correctly free relevant scripts when closing scene tabs Dec 18, 2023

Correctly free relevant scripts when closing scene tabs

b37b40f

Delsin-Yu force-pushed the 4.2 branch from 0010722 to b37b40f Compare December 18, 2023 13:41

YuriSizov merged commit c3ee2b9 into godotengine:master Dec 18, 2023
15 checks passed

Delsin-Yu deleted the 4.2 branch December 18, 2023 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly free relevant scripts when closing scene tabs #86008

Correctly free relevant scripts when closing scene tabs #86008

Delsin-Yu commented Dec 10, 2023

This comment was marked as outdated.

AThousandShips commented Dec 10, 2023

AThousandShips left a comment

Delsin-Yu commented Dec 10, 2023 •

edited

Loading

KoBeWi commented Dec 14, 2023

Delsin-Yu commented Dec 15, 2023

KoBeWi commented Dec 15, 2023

Delsin-Yu commented Dec 16, 2023

Delsin-Yu commented Dec 16, 2023

KoBeWi commented Dec 16, 2023

Delsin-Yu commented Dec 17, 2023 •

edited

Loading

KoBeWi left a comment

YuriSizov commented Dec 18, 2023

YuriSizov commented Dec 18, 2023

Correctly free relevant scripts when closing scene tabs #86008

Correctly free relevant scripts when closing scene tabs #86008

Conversation

Delsin-Yu commented Dec 10, 2023

This comment was marked as outdated.

AThousandShips commented Dec 10, 2023

AThousandShips left a comment

Choose a reason for hiding this comment

Delsin-Yu commented Dec 10, 2023 • edited Loading

Original Logic

New Logic that Causes Crashing

The Crash Message

KoBeWi commented Dec 14, 2023

Delsin-Yu commented Dec 15, 2023

KoBeWi commented Dec 15, 2023

Delsin-Yu commented Dec 16, 2023

Delsin-Yu commented Dec 16, 2023

KoBeWi commented Dec 16, 2023

Delsin-Yu commented Dec 17, 2023 • edited Loading

KoBeWi left a comment

Choose a reason for hiding this comment

YuriSizov commented Dec 18, 2023

YuriSizov commented Dec 18, 2023

Delsin-Yu commented Dec 10, 2023 •

edited

Loading

Delsin-Yu commented Dec 17, 2023 •

edited

Loading